Repository navigation
WW-5827 check the preparer type before instantiating it and do not cache failed preparer lookups - #2084
WW-5827 check the preparer type before instantiating it and do not cache failed preparer lookups#2084arunmanni-ai wants to merge 5 commits into
Conversation
…che failed preparer lookups ClassUtil.getClass() resolved a class with initialize=true, so the named class ran its static initialiser on lookup, and instantiate(String, boolean) passed Object.class as the base type, which left the asSubclass check unable to fail. BasicPreparerFactory therefore cast to ViewPreparer only after the named class had been loaded, initialised and constructed. The class is now resolved without being initialised, and a typed instantiate(String, Class, boolean) applies the caller's base type before the constructor runs, so a class that does not denote a ViewPreparer is rejected without anything being constructed. A name that resolves but does not conform is reported as CannotInstantiateObjectException rather than returned as null, which keeps the failure as visible as the ClassCastException it replaces; returnNull continues to mean "class not found" only. Resolution and construction are caught separately, so an exception thrown by the constructor itself is never reported as a type mismatch. BasicPreparerFactory.getPreparer() stored the result of createPreparer() unconditionally, so a null was retained and thereafter reported as resolved by containsKey(). That made a single failure permanent for the lifetime of the factory even when its cause was transient. Failed lookups are no longer recorded, and the map is a ConcurrentHashMap, since it is read and written by request threads. TilesDispatchServlet is the only other caller of the untyped helper and is migrated to the typed one.
|
Hi @lukaszlenart , One scope question. In a Struts deployment Two things would fold into the same change. The fallback logs at ERROR with a full stack trace on every call, and now that failed lookups are no longer cached, a misconfigured preparer name logs that per request rather than once. Should both go in this PR, or would you rather have a separate ticket? Happy either way — I just didn't want to widen this one without asking. |
lukaszlenart
left a comment
lukaszlenart
left a comment
There was a problem hiding this comment.
Thanks, this looks good: the scope matches WW-5827 and the tests cover both items. Two small things inline, neither blocking.
|
Dropped the inline comments in both files and the two “Prefer instantiate(String, Class, boolean)” paragraphs from the untyped methods’ javadoc. |
|
Thanks for the cleanup, and sorry for the slow answer on the scope question. Yes, please fold both into this PR. You're right about the logging: with failed lookups no longer cached, the ERROR plus stack trace from the One small thing from the last push: |
|
Both folded in, and the trailing whitespace is gone. Three judgement calls in the The
The non-conforming case throws rather than returning null. Same reasoning as the On the logging: the fallback is now WARN with the exception message and the stack trace at DEBUG, so a misconfigured name no longer produces an ERROR stack trace per request.
|
Fixes WW-5827
Addresses the three items in the ticket.
ClassUtil instantiates before checking the type
getClass(String, Class)resolved withinitialize=true, so the named class ran its static initialiser on lookup, andinstantiate(String, boolean)passedObject.classas the base type, which left theasSubclasscheck unable to fail.BasicPreparerFactorycast toViewPrepareronly afterwards, so a configured name that does not denote aViewPreparerwas loaded, initialised and constructed before it was rejected.The class is now resolved without being initialised, and a new
instantiate(String, Class<T>, boolean)applies the caller's base type before the constructor runs, so a name that does not conform is rejected without anything being constructed.A name that resolves but does not conform is reported as
CannotInstantiateObjectExceptionrather than returned asnull. Returningnullhere would be swallowed by theignoreMissingpath inBasicTilesContainer.prepare, turning a misconfigured preparer into a silent no-op; the exception keeps the failure as visible as theClassCastExceptionit replaces.returnNullcontinues to mean "class not found" only. Resolution and construction are caught separately, so an exception thrown by the constructor itself is never reported as a type mismatch.The untyped
instantiatemethods are unchanged;TilesDispatchServletis the only other caller and is migrated to the typed one.BasicPreparerFactory caches failed lookups in a non-thread-safe map
getPreparerstored whatevercreatePreparerreturned, includingnull, whichcontainsKeythen reported as resolved. A single failure became permanent for the lifetime of the factory even when its cause was transient. Failed lookups are no longer recorded, and the map is aConcurrentHashMap, since it is read and written by request threads.get/putIfAbsentrather thancomputeIfAbsent, so class loading does not run while a bin of the map is locked.StrutsPreparerFactory constructs before checking the type, twice on failure, and logs every failure at ERROR
createPreparerbuilt the preparer throughObjectFactory.buildBeanand cast toViewPreparerafterwards, so the check added above did not apply on this path. When the cast failed, thecatch (Exception)fell back tosuper.createPreparer, which constructed the class a second time. That fallback logged at ERROR with a full stack trace; with failed lookups no longer cached, a misconfigured name would log that on every request instead of once.The type is now checked before the bean is built, so a name that resolves to a class that is not a
ViewPrepareris rejected without anything being constructed.checkTyperesolves the name without initialising it and letsClassNotFoundExceptionpass, since a preparer name need not be a class name — bean names resolved by the object factory reachbuildBeantoo and are left to it. A name that does resolve but does not conform throwsCannotInstantiateObjectException, as in theClassUtilpath.A bean that is built and turns out not to be a
ViewPrepareris reported rather than passed to the fallback, so it is not constructed a second time. The fallback is kept for a genuinebuildBeanfailure, and now logs at WARN with the exception message; the stack trace moves to DEBUG.Tests
mvn test -DskipAssembly -pl plugins/tiles— 576 tests, 0 failures.New
BasicPreparerFactoryTypeCheckTestcovers rejection before initialisation and construction, non-retention of a name that cannot be resolved, and that a genuineViewPrepareris still created and cached. The evidence flags live in a separate holder class, because reading a static member of the marker class would itself initialise it. Four cases added toClassUtilTestfor the typed overload.New
StrutsPreparerFactoryTypeCheckTestcovers the third section: a name that resolves to a non-ViewPrepareris rejected beforebuildBeanis called, a bean that is built but does not conform is reported without a second construction, and a genuine build failure still falls back to the superclass and constructs once. TheObjectFactoryis a Mockito mock bound into a realActionContext.PatternUtilTestis unchanged.